StringComparer Create(culture, CompareOptions) overload #16334

Anipik · 2018-02-12T00:44:48Z

Related to https://github.com/dotnet/corefx/issues/395
Tests PR - dotnet/corefx#27051

danmoseley · 2018-02-12T01:12:01Z

src/mscorlib/shared/System/StringComparer.cs

+            {
+                throw new ArgumentException(nameof(culture));
+            }
+            return new CultureAwareComparer(culture, options);


nit newline, per the style above

danmoseley · 2018-02-12T01:18:55Z

src/mscorlib/shared/System/StringComparer.cs

+        internal CultureAwareComparer(CultureInfo culture, CompareOptions compareOptions)
+        {
+            _compareInfo = culture.CompareInfo;
+            _compareOptions = compareOptions;


It's a shame you have to keep _ignoreCase around as it's now redundant, but I agree you can't remove it due to serialization.

danmoseley · 2018-02-12T01:20:08Z

This seems reasonable but @tarekgh can you review? There are a forest of various comparers, compareinfo's, cultureinfos and so forth - can you confirm this makes sense.

note this APi was approved here
https://github.com/dotnet/corefx/issues/395#issuecomment-297115553

stephentoub · 2018-02-12T01:25:53Z

src/mscorlib/shared/System/StringComparer.cs

        private readonly CompareInfo _compareInfo; // Do not rename (binary serialization)
        private readonly bool _ignoreCase; // Do not rename (binary serialization)
+        private CompareOptions _compareOptions;



I believe adding this breaks serialization compat with desktop. It likely needs to be made Optional.

Good catch, yes it doesn't implement ISerializable. @ViktorHofer I guess this is the first point we have to consider cross version serialization from 2.0. Should we use [OptionalField] (with a Version?) and have the code check _ignoreCase first? Or switch this to ISerializable (which I assume could be done non breaking, but I don't know.) so we can set it to match _ignoreCase.

Also @ViktorHofer do we have a test strategy for 2.0<->2.1 serialization interop?

Another option is one of the [De]Serializ[ing|ed] attributes, but IIRC there was some issue with those that we fixed recently (?)

@Anipik if you're not famliar with binary serialization, read all these pages including this one that is relevant here

Correct, the following would happen:

Core 2.1 --> Core2.0/NETFX: pass, additional field in the blob is ignored
NETFX/Core2.0 --> Core 2.1: throw, missing field in the blob

The easiest solution is as Stephen stated mark _compareOptions as optional. IMO the best solution would be to mark the type as ISerializable, get rid of the _ignoreCase field and during deserialization try to read _compareOptions from the serialization blob with GetValueNoThrow and if that fails read _ignoreCase from the blob. For serialization inside GetObjectData, both fields would needed to be written to the blob.

to clarify, ISerializable is usually the old way used in serialization and it is used more if you are changing the field names. anyway, we need to wait a little bit to tell what is the best way we should go with.

one more thing, OptionalField is very important to have, otherwise will break deserialization on old desktop versions (4.7 for instance)

We are using it on plenty of types in the BCL, e.g. on all Exception types when we need more control over the serialization process. In this case it makes sense as we can get rid of the extra bool fields which benefits all scenarios, even when you don't care about serialization at all

we can get rid of the extra bool fields which benefits all scenarios

This will break the deserialization on on the full framework. except if you are talking to manually store it when we are serializing it. either way, we have to handle it.

Yes, I mean to manually store it, that's why I'm pushing for ISerializable.

ViktorHofer · 2018-02-12T14:21:05Z

src/mscorlib/shared/System/StringComparer.cs

+            _compareOptions = compareOptions;
+        }
+
+        private CompareOptions Options


this property isn't needed. Just passt the _options field to the CompareInfo.Compare calls

I agree with Viktor. this property is not needed

In reply to: 167567810 [](ancestors = 167567810)

ViktorHofer · 2018-02-12T14:23:24Z

src/mscorlib/shared/System/StringComparer.cs

            return new CultureAwareComparer(culture, ignoreCase);
        }

+        public static StringComparer Create(CultureInfo culture, CompareOptions options)


If we introduce this new API I would expect the existing one static StringComparer StringComparer.Create(CultureInfo, bool) to be marked as obsolete.

@danmosemsft should I mark this as obsolete ?

No. it is still ok to use

We do not mark new API with the obsolete attribute, because historically it makes it hard for customers with large codebases and "warnings as errors" to upgrade their platforms. Instead you could open an issue in https://github.com/dotnet/platform-compat to flag it dynamically.

Yes I wasn't talking about ObsoleteAttribute but about the compat tooling.

tarekgh · 2018-02-12T16:37:43Z

please wait don't merge this one.

tarekgh · 2018-02-12T16:51:44Z

This functionality is already supported in

https://github.com/dotnet/corefx/blob/master/src/System.Globalization.Extensions/src/System/Globalization/Extensions.cs#L12

So you can do it through

CompareInfo.GetStringComparer(options)

We have 2 copies of CultureAwareComparer implementations now. would be better if we think to unify that and keep just one copy in coreclr and change corefx to call the coreclr one.

For serialization, as @stephentoub pointed early this will break the desktop serialization. to fix that, will need to mark the newly introduced field as [OptionalField]

and then add the following method

        [OnDeserialized]
        private void OnDeserializedMethod(StreamingContext context)
        {
            _options |= _ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None;
        }

tarekgh · 2018-02-12T16:52:24Z

cc @AlexGhiondea @krwq

tarekgh · 2018-02-12T16:57:05Z

src/mscorlib/shared/System/StringComparer.cs

            return new CultureAwareComparer(culture, ignoreCase);
        }

+        public static StringComparer Create(CultureInfo culture, CompareOptions options)


CompareOptions options [](start = 65, length = 22)

we should validate the options here

tarekgh · 2018-02-13T01:22:56Z

src/mscorlib/shared/System/StringComparer.cs

+            }
+
+            if (!Enum.IsDefined(typeof(CompareOptions), options))
+            {


we should check the options here as we are doing it in https://github.com/dotnet/corefx/blob/master/src/System.Globalization.Extensions/src/System/Globalization/Extensions.cs#L29

the reason is we shouldn't allow ordinal options here

why shouldn't we allow ordinal here ?

because if anyone want to use ordinal option should create OrdinalComparer and not culture aware comparer

private static readonly OrdinalCaseSensitiveComparer s_ordinal = new OrdinalCaseSensitiveComparer(); private static readonly OrdinalIgnoreCaseComparer s_ordinalIgnoreCase = new OrdinalIgnoreCaseComparer();

In reply to: 167785331 [](ancestors = 167785331)

tarekgh · 2018-02-13T01:30:45Z

src/mscorlib/shared/System/StringComparer.cs

+        private void OnDeserializedMethod(StreamingContext context)
+        {
+            _options |= _ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None;
+        }


I know I have suggested this way but I was looking same time at issue on the full framework. after investigating it I found it is better to have CultureAwareComparer implement ISerializable something like

public CultureAwareComparer(SerializationInfo info, StreamingContext context) { _compareInfo = (CompareInfo) info.GetValue("_compareInfo", typeof(CompareInfo)); bool ignoreCase = info.GetBoolean("_ignoreCase"); var obj = info.GetValueNoThrow("_options", typeof(CompareOptions)); if (obj != null) _options = (CompareOptions) obj; // fix up the _options value in case we are getting old serialized object not having _options _options |= ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None; } public void GetObjectData(SerializationInfo info, StreamingContext context) { info.AddValue("_compareInfo", _compareInfo); info.AddValue("_options", _options); info.AddValue("_ignoreCase", _(options & CompareOptions.IgnoreCase) != 0); }

note that you can fully remove _ignoreCase now too.

ViktorHofer · 2018-02-13T14:32:22Z

src/mscorlib/shared/System/StringComparer.cs

        private readonly CompareInfo _compareInfo; // Do not rename (binary serialization)
-        private readonly bool _ignoreCase; // Do not rename (binary serialization)

+        [OptionalField]


If the type implements ISerializable and with that takes control of the serialization process, there is no need for an OptionalFieldAttribute here

ViktorHofer · 2018-02-13T14:34:51Z

src/mscorlib/shared/System/StringComparer.cs

+            _options = compareOptions;
+        }
+
+        internal CultureAwareComparer(SerializationInfo info, StreamingContext context)


This should be private as this constructor will only be invoked by the serialization engine via reflection and because the type is sealed

jkotas · 2018-02-13T15:46:29Z

src/mscorlib/shared/System/StringComparer.cs

        {
            int hashCode = _compareInfo.GetHashCode();
-            return _ignoreCase ? ~hashCode : hashCode;
+            return (_options & CompareOptions.IgnoreCase) != 0 ? ~hashCode : hashCode;


hashCode should include all _options, not just ignore case.

jkotas · 2018-02-13T15:49:08Z

src/mscorlib/shared/System/StringComparer.cs

+        [OptionalField]
+        private CompareOptions _options;
+
        internal CultureAwareComparer(CultureInfo culture, bool ignoreCase)


This constructor is not needed. The callers can use the other one and just pass in the right compare options.

tarekgh · 2018-02-13T16:37:44Z

src/mscorlib/shared/System/StringComparer.cs

+            if ((options & CultureAwareComparer.ValidCompareMaskOffFlags) != 0)
+            {
+                throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options));
+            }


onen last comment: you may move this check to the CultureAwareComparer constructor so you don't have to use ValidCompareMaskOffFlags here and everything will be well contained inside CultureAwareComparer

tarekgh · 2018-02-13T16:40:31Z

other than the mentioned comments, LGTM. thanks @Anipik. please don't forget when you expose the new API in the corefx, we'll need to get rid of the duplicate CultureAwareComparer implementation there.

jkotas · 2018-02-13T16:55:02Z

we'll need to get rid of the duplicate CultureAwareComparer implementation there.

That will require moving System.Globalization.GlobalizationExtensions to CoreLib, right?

tarekgh · 2018-02-13T17:05:12Z

That will require moving System.Globalization.GlobalizationExtensions to CoreLib, right?

No, we don't need to move anything to coreclr, we'll just need to change the implementation of

public static StringComparer GetStringComparer(this CompareInfo compareInfo, CompareOptions options)

to use the newly exposed API.

https://github.com/dotnet/corefx/blob/master/src/System.Globalization.Extensions/src/System/Globalization/Extensions.cs#L12

tarekgh · 2018-02-13T20:18:08Z

src/mscorlib/shared/System/StringComparer.cs

+            if (ignoreCase)
+                return new CultureAwareComparer(culture, CompareOptions.IgnoreCase);
+
+            return new CultureAwareComparer(culture, CompareOptions.None);


nit: maybe you can do it in one line using ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None;

tarekgh · 2018-02-13T20:19:27Z

src/mscorlib/shared/System/StringComparer.cs

+        private CultureAwareComparer(SerializationInfo info, StreamingContext context)
+        {
+            _compareInfo = (CompareInfo)info.GetValue("_compareInfo", typeof(CompareInfo));
+            bool ignoreCase = info.GetBoolean("ignoreCase");


ignoreCase [](start = 47, length = 10)

you are missing the underscore prefix. should be _ignoreCae

tarekgh · 2018-02-13T21:23:54Z

just to tell we can fully get rid of the implementation in corefx as currently this implementation is used for netfx only and we are not building netfx packages for System.Globalization.Extensions anymore.

ViktorHofer

The changes look good to me but before you merge this I want you to verify that this doesn't break compatibility with netfx/netcoreapp2.0. Please add a test in System.Runtime.Seriazation.Formatters.Tests and run it with your local built coreclr package based on this commit.

Anipik · 2018-02-13T23:13:05Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

jkotas · 2018-02-14T07:23:15Z

src/mscorlib/shared/System/StringComparer.cs

                throw new ArgumentNullException(nameof(culture));
            }
+
+             return new CultureAwareComparer(culture, ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None);


Nit: Indentation is one space off.

jkotas · 2018-02-14T07:29:08Z

we can fully get rid of the implementation in corefx as currently this implementation is used for netfx

Note that there is another duplicate implementation at
https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/src/System/Globalization/Extensions.cs that is used on .NET Core. It would be nice to dedup it with this one...

jkotas · 2018-02-14T07:37:34Z

No, we don't need to move anything to coreclr, we'll just need to change the implementation of
public static StringComparer GetStringComparer(this CompareInfo compareInfo, CompareOptions options)
to use the newly exposed API.

@tarekgh The newly exposed API takes CultureInfo; but this API takes CompareInfo. I do not think there is a good way to map CompareInfo back to CultureInfo.

tarekgh · 2018-02-14T20:24:30Z

The newly exposed API takes CultureInfo; but this API takes CompareInfo. I do not think there is a good way to map CompareInfo back to CultureInfo.

we don't need the other implementation at all. so we should be fine here. Also we can get the culture object from the compareinfo object using CultureInfo.GetCultureInfo(compareInfo.Name)

jkotas · 2018-02-14T20:48:41Z

Also we can get the culture object from the compareinfo object using CultureInfo.GetCultureInfo(compareInfo.Name)

If GlobalizationExtensions.GetStringComparer starts doing this, it will become order of magnitude slower compare to what it is today...

tarekgh · 2018-02-14T20:58:59Z

If GlobalizationExtensions.GetStringComparer starts doing this, it will become order of magnitude slower compare to what it is today...

CultureInfo.GetCultureInfo is the cached version which should be good enough. the other idea is to expose override that work with the compareinfo from coreclr too. Also, this extension method is used only for who want to call CompareInfo.GetStringComparer which is not used in all our collection code nor is popular and people should use CultureAwareComparer and this extension.

ViktorHofer

Anirudh verified locally that serialization is still working with netfx/netcoreapp2.0.

Anipik · 2018-02-14T21:52:25Z

@jkotas @tarekgh should I merge it now ?

tarekgh · 2018-02-14T22:07:58Z

I am fine with the changes

tarekgh · 2018-02-15T01:40:03Z

@jkotas if you are ok with changes, we can merge this one.

ViktorHofer · 2018-02-22T21:37:46Z

src/mscorlib/shared/System/StringComparer.cs

+
        private readonly CompareInfo _compareInfo; // Do not rename (binary serialization)
-        private readonly bool _ignoreCase; // Do not rename (binary serialization)
+        private CompareOptions _options;


that one should be readonly

I just checked with net471, they named the field the same. lucky us.
https://referencesource.microsoft.com/#mscorlib/system/stringcomparer.cs,140

tarekgh · 2018-02-22T21:41:43Z

I just checked with net471, they named the field the same. lucky us.

It is not by luck. we have checked that.

TylerBrinkley · 2019-09-06T19:18:06Z

src/mscorlib/shared/System/StringComparer.cs

+        {
+            if (culture == null)
+            {
+                throw new ArgumentException(nameof(culture));


I know this PR is extremely old but why doesn't this throw an ArgumentNullException?

@TylerBrinkley I think it is a mistake. Thanks for pointing it out. Generally it's best to open a new issue (or just PR) since posting on closed issues/PR's sometimes gets overlooked.

Did somebody follow-up on this? Was an issue created?

I created #26570 to address this.

Anipik added 2 commits February 10, 2018 21:57

String Create options overlaod

4591869

Minor Change

7606c3c

Anipik requested a review from danmoseley February 12, 2018 00:44

Anipik mentioned this pull request Feb 12, 2018

Tests for StringComparer Create(culture, CompareOptions) overload dotnet/corefx#27051

Merged

danmoseley reviewed Feb 12, 2018

View reviewed changes

stephentoub reviewed Feb 12, 2018

View reviewed changes

ViktorHofer reviewed Feb 12, 2018

View reviewed changes

tarekgh added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 12, 2018

tarekgh reviewed Feb 12, 2018

View reviewed changes

Anipik added 3 commits February 12, 2018 14:38

Feedback

aa21f43

validation on options

2904680

obsolete removed

23acf6c

tarekgh reviewed Feb 13, 2018

View reviewed changes

Implementing Iserializable and removing ignorecase

cbb560a

ViktorHofer reviewed Feb 13, 2018

View reviewed changes

jkotas reviewed Feb 13, 2018

View reviewed changes

tarekgh reviewed Feb 13, 2018

View reviewed changes

tarekgh removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 13, 2018

HashCode and serialization changes

d279555

Anipik mentioned this pull request Feb 13, 2018

Removed duplicated Implementation of CultureAwareComparer dotnet/corefx#27099

Merged

tarekgh reviewed Feb 13, 2018

View reviewed changes

made inline

f143205

tarekgh approved these changes Feb 13, 2018

View reviewed changes

ViktorHofer suggested changes Feb 13, 2018

View reviewed changes

jkotas reviewed Feb 14, 2018

View reviewed changes

Space Corrected

d592c7a

ViktorHofer approved these changes Feb 14, 2018

View reviewed changes

jkotas approved these changes Feb 15, 2018

View reviewed changes

tarekgh merged commit 7995334 into dotnet:master Feb 15, 2018

ViktorHofer reviewed Feb 22, 2018

View reviewed changes

Anipik deleted the stringComparer branch March 24, 2018 01:35

TylerBrinkley reviewed Sep 6, 2019

View reviewed changes

StringComparer Create(culture, CompareOptions) overload #16334

StringComparer Create(culture, CompareOptions) overload #16334

Uh oh!

Conversation

Anipik commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Feb 12, 2018

Uh oh!

tarekgh commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarekgh commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Feb 13, 2018

Uh oh!

jkotas commented Feb 13, 2018

Uh oh!

Anipik commented Feb 12, 2018 •

edited

Loading

tarekgh commented Feb 12, 2018 •

edited

Loading

tarekgh Feb 13, 2018 •

edited

Loading